Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provides ExitCode Shutdowner Option; and Wait method to receive it. #989

Merged
merged 33 commits into from
Dec 7, 2022

Conversation

jasonmills
Copy link
Contributor

This PR provides an option for those who take dependencies on the Shutdowner interface to call the Shutdown method with an ExitCode option, in addition it add a Wait method to the application to allow for main programs to wait for the application to be shutdown and to exit with a given exit code.

Please note that this PR refactors the existing signal relay functionality, and alters application lifecycle slightly. Now Done will not receive an os.Signal on the channel it returns unless a given FX application has been started.

@jasonmills jasonmills marked this pull request as ready for review November 15, 2022 22:46
Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a first pass. Looking good!

app.go Outdated Show resolved Hide resolved
shutdown.go Outdated Show resolved Hide resolved
app.go Outdated Show resolved Hide resolved
app.go Outdated Show resolved Hide resolved
app.go Outdated Show resolved Hide resolved
shutdown_test.go Outdated Show resolved Hide resolved
shutdown.go Outdated Show resolved Hide resolved
signal.go Outdated Show resolved Hide resolved
signal_test.go Outdated Show resolved Hide resolved
shutdown_test.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Merging #989 (2e6dd3a) into master (94f1a09) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #989      +/-   ##
==========================================
+ Coverage   98.32%   98.39%   +0.07%     
==========================================
  Files          39       39              
  Lines        1908     1994      +86     
==========================================
+ Hits         1876     1962      +86     
  Misses         25       25              
  Partials        7        7              
Impacted Files Coverage Δ
app.go 96.41% <100.00%> (+0.17%) ⬆️
shutdown.go 100.00% <100.00%> (ø)
signal.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

annotated_test.go Outdated Show resolved Hide resolved
shutdown.go Outdated Show resolved Hide resolved
shutdown.go Outdated Show resolved Hide resolved
signal.go Outdated Show resolved Hide resolved
signal.go Show resolved Hide resolved
jasonmills and others added 8 commits December 7, 2022 21:48
Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>
Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>
Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>
Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>
@jasonmills jasonmills merged commit b379e13 into uber-go:master Dec 7, 2022
sywhang added a commit that referenced this pull request Jan 9, 2023
This fixes #1015.

When Start() is called, it passes StartTimeout to the relayer goroutine,
which is monitoring the Stop signal, as well as the OS signal handler.

It then exits the goroutine if the start context expires, which would
naturally happen if the user uses the default setting (15s timeout) or
any finite amount of timeout value.

This causes the app to then be not responsive to any OS signals such as
SIGTERM or SIGINT.

This was a regression introduced in 1.19 release via #989 .

To fix this, this commit simply removes the relayer goroutine from
selecting on the start context being completed.

Verified that all tests are still passing without any goroutine leak,
except one test that was triggering a panic to test the panic handler.

To prevent that one test from opting out every single test into
goleak.VerifyNone(t) in every sub test, I pulled out that panic test
into a separate test package so that we can continue to use
goleak.VerifyNone() method in app_test.
abhinav added a commit to abhinav/fx that referenced this pull request Apr 29, 2023
We added support for changing the exit code for a Shutdowner with the
ExitCode option in uber-go#989, but this was somehow not respected by App.Run.

This changes App.Run to use the same underlying machinery (`Wait()`)
to decide on the exit code to use.

Resolves uber-go#1074
abhinav added a commit to abhinav/fx that referenced this pull request Apr 29, 2023
We added support for changing the exit code for a Shutdowner with the
ExitCode option in uber-go#989, but this was somehow not respected by App.Run.

This changes App.Run to use the same underlying machinery (`Wait()`)
to decide on the exit code to use.

Resolves uber-go#1074
sywhang pushed a commit that referenced this pull request Apr 29, 2023
We added support for changing the exit code for a Shutdowner with the
ExitCode option in #989, but this was somehow not respected by App.Run.

This changes App.Run to use the same underlying machinery (`Wait()`) to
decide on the exit code to use.

To test this, we add a new internal/e2e submodule that will hold full,
end-to-end integration tests.
These can be full Fx applications that we run tests against.
This is a submodule so that it can have dependencies that are not
desirable as direct dependencies of Fx,
and it's inside the internal/ directory so that it can consume
Fx-internal packages (like testutil).

The included regression test verifies the behavior described in #1074.
An Fx program using App.Run, and shut down with Shutdowner.Shutdown
and an explicit exit code, does not exit with the requested exit code.
Failure before the fix:

```
% go test
--- FAIL: TestShutdownExitCode (0.01s)
    writer.go:40: [Fx] PROVIDE  fx.Lifecycle <= go.uber.org/fx.New.func1()
    writer.go:40: [Fx] PROVIDE  fx.Shutdowner <= go.uber.org/fx.(*App).shutdowner-fm()
    writer.go:40: [Fx] PROVIDE  fx.DotGraph <= go.uber.org/fx.(*App).dotGraph-fm()
    writer.go:40: [Fx] INVOKE           go.uber.org/fx/internal/e2e/shutdowner_run_exitcode.main.func1()
    writer.go:40: [Fx] RUNNING
    writer.go:40: [Fx] TERMINATED
    main_test.go:46:
                Error Trace:    [..]/fx/internal/e2e/shutdowner_run_exitcode/main_test.go:46
                Error:          An error is expected but got nil.
                Test:           TestShutdownExitCode
FAIL
exit status 1
FAIL    go.uber.org/fx/internal/e2e/shutdowner_run_exitcode     0.016s
```

Resolves #1074

---

There's a follow up to this: abhinav#1.
It depends on the e2e test machinery, so I'll make a PR out of it once
this is merged.
JacobOaks pushed a commit that referenced this pull request Jun 25, 2024
closes #1212, and fixes a regression from #989. Previously we would only
register signal handlers if the user intended to use them. #989 changed
this behavior
[here](https://github.com/uber-go/fx/pull/989/files#diff-6c4b6ed7dc8834cef100f50dae61c30ffe7775a3f3f6f5a557517cb740c44a2dR649).

This regression meant that if you only used app.Start()/app.Stop(), fx
would register signal handlers for no reason as the user didn't use
app.Done/app.Wait.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants